Skip to content

fix(mcp): read the initialize handshake before the transport consumes the body - #10191

Merged
JSONbored merged 1 commit into
mainfrom
fix/mcp-initialize-body-clone-10190
Jul 31, 2026
Merged

fix(mcp): read the initialize handshake before the transport consumes the body#10191
JSONbored merged 1 commit into
mainfrom
fix/mcp-initialize-body-clone-10190

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Every MCP initialize request has been returning 500 since #10175 landed. The handshake telemetry it added called c.req.raw.clone() after createMcpHandler had already consumed the request body:

  • The Fetch spec forbids cloning a request whose body is disturbed, so readInitializeHandshake threw TypeError: unusable.
  • handleMcpRequest's catch block rethrows, so a correct 2xx MCP response was discarded and replaced by an unhandled_route_error → 500.
  • The telemetry gate did not contain it. recordMcpInitialize no-ops without POSTHOG_API_KEY, but the handshake read is an argument to it and is evaluated first — the throw happened regardless of configuration.

initialize is the first call of every MCP session, so no client could complete a handshake. tools/list was unaffected (recordMcpToolsList reads the server's own registered tool names and never touches the body).

The fix parses the JSON-RPC envelope once, before the handler runs, and derives both the usage metadata and the clientInfo handshake from that single parse. describeMcpUsageRequest and readInitializeHandshake become pure functions over that envelope, so no clone can survive past the handler again.

Closes #10190

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • Coverage was measured scoped to the changed file rather than via a full test:coverage run: 100% of the changed lines and branches in src/mcp/server.ts are covered (verified against the lcov report for test/unit/mcp-server-telemetry.test.ts + test/integration/api.test.ts). The unchecked commands are untouched surfaces — this diff changes no workflow, worker binding, OpenAPI schema, MCP package manifest, or UI file — and are left to CI.
  • Both new tests were confirmed to fail with the fix reverted (TypeError: unusable) and pass with it, so they pin the regression rather than merely passing.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

Not applicable — no visible UI, frontend, docs, or extension change. This is a server-side request-handling fix.

Notes

The existing integration test test/integration/api.test.ts > api routes > serves private MCP tool listing and tool calls was already failing on main at its expect(telemetryDownInitialize.status).toBe(200) assertion; it passes again with this change. The two added unit tests cover the handshake path directly, including an initialize with no params and one whose clientInfo fields are not strings — both must still succeed, since those fields are optional by contract and a client must never be failed over LoopOver's own instrumentation.

… the body

The #10175 handshake telemetry called `c.req.raw.clone()` after createMcpHandler
had already read the request body. The Fetch spec forbids cloning a request whose
body is disturbed, so it threw `TypeError: unusable`; handleMcpRequest's catch
rethrew, and a correct 2xx MCP response was discarded in favour of an unhandled
500 -- on every `initialize`, the first call of every MCP session.

The telemetry key gate did not contain it: the handshake read is an argument to
recordMcpInitialize, evaluated before that function's own no-op check, so the
throw happened whether or not POSTHOG_API_KEY was set.

Parse the JSON-RPC envelope once, before the handler runs, and derive both the
usage metadata and the clientInfo handshake from it. No clone survives past the
handler.

Closes #10190
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Important

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏳ LoopOver is waiting…

LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 31, 2026
@JSONbored
JSONbored merged commit f1a0a32 into main Jul 31, 2026
4 checks passed
@JSONbored
JSONbored deleted the fix/mcp-initialize-body-clone-10190 branch July 31, 2026 11:55
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.34%. Comparing base (693aed8) to head (0306324).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##             main   #10191       +/-   ##
===========================================
+ Coverage   80.44%   91.34%   +10.89%     
===========================================
  Files         282      934      +652     
  Lines       58764   114145    +55381     
  Branches     6965    27585    +20620     
===========================================
+ Hits        47274   104267    +56993     
+ Misses      11199     8771     -2428     
- Partials      291     1107      +816     
Flag Coverage Δ
backend 94.13% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/mcp/server.ts 97.69% <100.00%> (ø)

... and 783 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mcp: every initialize returns 500 — the handshake telemetry clones the request body after the handler consumed it

1 participant